Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy Paste Data #2928

Merged
merged 17 commits into from
Dec 20, 2023
Merged

Copy Paste Data #2928

merged 17 commits into from
Dec 20, 2023

Conversation

jameskerr
Copy link
Member

@jameskerr jameskerr commented Dec 11, 2023

Closes #2909

  • Integration Test
  • Unique File Name
  • Delete File after Loading
  • Allow Pasting More and More data into Preview And Load
  • Read the directory for the files names, don't store the this.files array
  • Change DropFilesTarget to use Dialog and ShowModal so that it will go to the top of the screen.
  • Destroy the folder when the app quits

Bugs:

  • Creating the temp dir needs to work
  • The end to end test ends too fast before the ingest finishes.

Copy link
Contributor

@philrz philrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Some suggestions before we merge.

  1. Could we have a simple e2e test for this? I'd like to avoid accumulating more features that we'll only be able to catch manually if they regress.

  2. It looks like the temporary file created to hold the contents of the paste buffer is left behind after the operation. Can we make sure that gets deleted? I know we've rationalized things like leaving behind the temp files we create for pcap extraction because users might keep working on them in Wireshark, but for this case I don't see a reason to let them accumulate (and we've had users come to us concerned about droppings we've left behind in temp). My bigger concern is that there might still be environments (particularly Linux) where multiple desktop users live off the same host. Right now when running with defaults on Linux, Zui's temporary files land in /tmp, so it seems like two different users both using this feature have the potential to step on each other's /tmp/clipboard.txt (whether that be one user's temp file overwriting someone else's or one user's temp file not able to be created because they lack the permissions to stomp the other user's temp file).

  3. Instead of the paste buffer always going to a static file called clipboard.txt, could we use one of those functions that's guaranteed to make a temp file with a unique name? Even if we do what I suggested in my prior bullet I can imagine scenarios (e.g., crashes) where temp files would still get left behind and hence leave the potential for things like I described in the prior bullet.

  4. The way it's working now I noticed that if I start Preview & Load based on the contents of the paste buffer, I can click "Add" and lump on more files from disk to the single load operation, which is pretty cool (see video below). Unfortunately it seems I can't do the reverse, i.e., initiate Preview & Load with one or more regular files and then lump on the paste buffer contents in addition to that, since if I'm at the Preview & Load screen and invoke "Paste Data..." it replaces all the files currently staged for load just like if I dragged a regular file into an already-open Preview & Load screen. I expect this feature will primarily be used for one-off pastes of data like spreadsheets similar to how we've been testing it so it probably doesn't make sense to do anything fancy like add a separate +Add Paste Buffer button on the Preview & Load screen. But I thought I'd point it out just in case.

Demo.mp4

@jameskerr jameskerr requested a review from philrz December 18, 2023 21:28
@jameskerr jameskerr merged commit 535f6e9 into main Dec 20, 2023
3 checks passed
@jameskerr jameskerr deleted the copy-paste-data branch December 20, 2023 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data import directly from paste buffer
2 participants